kernel_cmdline: Simplify Display impl for utf8::Parameter #1749
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request simplifies the Display implementation for utf8::Parameter to correctly return the original string slice, which is a great improvement for both correctness and clarity. The change ensures that quoting is preserved, and the new test case effectively validates this behavior. I have one minor suggestion to make the implementation even more idiomatic.
jeckersb
force-pushed
the
parameter_display
branch
from
601ba3c to
5df9ab8
Compare
| // SAFETY: We know this is valid UTF-8 since we only | ||
| // construct the underlying `bytes` from valid UTF-8 | ||
| str::from_utf8(&self.0).expect("We only construct the underlying bytes from valid UTF-8") |
There was a problem hiding this comment.
from_utf8 is a safe operation right? Isn't it from_utf8_unchecked that is the unsafe method?
Safe:
https://doc.rust-lang.org/std/primitive.str.html#method.from_utf8
If you are sure that the byte slice is valid UTF-8, and you don’t want to incur the overhead of the validity check, there is an unsafe version of this function, from_utf8_unchecked, which has the same behavior but skips the check.
Unsafe:
https://doc.rust-lang.org/std/str/fn.from_utf8_unchecked.html
Just wondering if the safety comment is necessary here. Seems like it would only be necessary if using the unchecked version.
There was a problem hiding this comment.
Yeah it's safe and probably not correct to the letter of the law of rust conventions to label it like that. It's like that because in the very earliest version of this code, it did actually use from_utf8_unchecked. But really it's not so performance critical that we need to squeeze out a few cycles and avoid the safe code. So we switched it to do the checked version and take the performance "penalty". In theory however, those comments still hold true and we should never hit the case where the underlying bytes are invalid UTF-8 and if we really really wanted to we could switch back to using the unchecked version.
Or put another way... it should be 100% impossible for this to panic. But if something does goes wrong, at least it will panic instead of undefined behavior.
Signed-off-by: John Eckersberg <[email protected]>
Really we want to just return the original slice that the Parameter was created from. This also aligns with how Cmdline and ParameterKey impl Display. Where this really matters the most is to ensure we retain the quoting that the parameter was created with, so I added a test just to sanity check that. Before this change the test would fail because "foo" would be stripped of its quotes and just rendered as foo unquoted. Signed-off-by: John Eckersberg <[email protected]>
henrywang
force-pushed
the
parameter_display
branch
from
5df9ab8 to
fcc9258
Compare
|
Just make a rebase for |
|
Hm not sure what the testing farm failures on stream 10 are about: Unrelated to this change but worth noting. |
4 participants
Really we want to just return the original slice that the Parameter
was created from. This also aligns with how Cmdline and ParameterKey
impl Display.
Where this really matters the most is to ensure we retain the quoting
that the parameter was created with, so I added a test just to sanity
check that. Before this change the test would fail because "foo"
would be stripped of its quotes and just rendered as foo unquoted.
Signed-off-by: John Eckersberg [email protected]